#1 moved auth parsing to signin method - #5
Conversation
shinchris
changed the title
|
🚀 means ship it. Make sure with the green merge button, click the down arrow and select Squash and Merge for the pull request. It's less important with this checkin, but if there is code review feedback, this will allow you to show a single merge. |
| site_id = parsed_response.find('.//t:site', namespaces=NAMESPACE).get('id', None) | ||
| user_id = parsed_response.find('.//t:user', namespaces=NAMESPACE).get('id', None) | ||
| auth_token = parsed_response.find('t:credentials', namespaces=NAMESPACE).get('token', None) | ||
| self.parent_srv._set_auth(site_id, user_id, auth_token) |
There was a problem hiding this comment.
Mostly style, but if this is getting called from outside the class/module, maybe it isn't really 'private' and doesn't need the '_' prefix.
There was a problem hiding this comment.
Our convention is any function call that is not intended for users to use should start with _. In this case, I think this fits that definition. @shinchris Do the users of the library ever have a handle to the endpoints? I don't remember
There was a problem hiding this comment.
Users can access the endpoints through the server class. Is that what you're asking?
Also I used the _ prefix because it's not intended for users to set the site_id, user_id, and auth_token.
There was a problem hiding this comment.
Makes sense -- I do like the convention, wasn't sure if it was supposed to be user-facing or not.
There was a problem hiding this comment.
@shinchris That's exactly what I was asking :) I thought that was the case, but couldn't remember off the top of my head. 🚀 🚀 🚀
|
🚀 Two small things |
…rty setter Two related fixes so unmapped auth values fail loudly at CSV parse time rather than producing a UserItem with silently missing auth_setting: - create_user_from_line: raise ValueError instead of silently setting auth to None when the AUTH column value isn't in _auth_canonical(). - _set_values: route auth_setting through the @property_is_enum(Auth) setter rather than writing to _auth_setting directly, so any invalid auth string is rejected at assignment. These two together close bug #5 in #1809 (setter bypass) and the silent- None finding surfaced in an adversarial review of the earlier commits on this branch. Callers who want lenient behavior (skip invalid rows, keep going) can catch the exception in their own iteration loop — that's the model tabcmd uses today via its --complete/--no-complete flag. Once this lands, tabcmd can defer its per-line validation to TSC (see #1809 and #1836). Also tightens test_too_many_columns_raises to expect ValueError only (was accepting either ValueError or AttributeError).
3 participants
No description provided.